fix: guard formatNumber against non-numeric values by themavik · Pull Request #1721 · ClickHouse/ClickStack

themavik · 2026-02-10T03:48:36Z

Summary

Root cause: When a number format (e.g., Percent) is applied to table columns that contain non-numeric values, formatNumber doesn't catch non-empty strings. numbro() then produces NaN.

The existing check !value && value !== 0 catches null, undefined, and NaN, but NOT non-empty strings (which are truthy and pass through to numbro()).

Changes

packages/app/src/utils.ts: Added typeof value !== 'number' || isNaN(value) guard that returns the raw value as a string instead of formatting it.

Risk Assessment

Low - Only adds a guard for an edge case. Numeric values are formatted exactly as before.

When table columns contain non-numeric values (strings, etc.) and a number format like Percent is applied, the formatting produces NaN because `numbro()` can't parse strings. The existing falsy check (`!value && value !== 0`) doesn't catch non-empty strings since they're truthy. Adds a `typeof` and `isNaN` guard to return the raw value as-is when it's not actually numeric. Fixes #1510

changeset-bot · 2026-02-10T03:48:40Z

🦋 Changeset detected

Latest commit: 5a572ca

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@hyperdx/app	Patch
@hyperdx/api	Patch
@hyperdx/otel-collector	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

vercel · 2026-02-10T03:48:41Z

@themavik is attempting to deploy a commit to the HyperDX Team on Vercel.

A member of the Team first needs to authorize it.

vercel · 2026-02-17T08:04:52Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
hyperdx-v2-oss-app	Ready	Preview, Comment	Feb 17, 2026 8:08am

MikeShi42 · 2026-02-19T22:14:23Z

@themavik I think we should only guard for isNaN - as it is possible for ClickHouse to return number values encoded as string due to https://clickhouse.com/docs/operations/settings/formats#output_format_json_quote_64bit_integers

karl-power

IMO this change doesn't address the root cause of the attached issue.

The problem is that grouping columns can exist which aren't found in the groupByKeys array here. The number formatter is then applied to these columns and we see NaN.

That can happen by adding a group by like ServiceName as SN or ResourceAttributes['host.id'] and the resulting column name (SN and arrayElement(ResourceAttributes, 'host.id') respectively) is not found in the groupByKeys array.

A solution would be to extend the code that creates the groupByKeys to check if aliases are used, and add the alias only to the array.

- Remove typeof check; ClickHouse returns numbers as strings - Only guard for isNaN to catch truly non-numeric values Co-authored-by: Cursor <cursoragent@cursor.com>

themavik · 2026-02-20T17:56:55Z

Thanks for the feedback. I've narrowed the guard to only check isNaN as @MikeShi42 suggested, since ClickHouse can return numbers as strings.

@karl-power — good point about groupByKeys not handling aliases. The alias handling could be a good follow-up improvement; this PR focuses on the immediate formatting crash.

github-actions · 2026-02-20T17:58:01Z

Code Review

⚠️ Root cause not fixed → groupByKeys in DBTableChart.tsx:115 splits the raw groupBy string but doesn't extract aliases (e.g. ServiceName as SN → key is SN, but groupByKeys contains ServiceName as SN), so groupByKeys.includes(key) returns false for aliased columns and numberFormat is incorrectly applied. This PR is a defensive patch, but the alias handling should also be fixed to prevent the misconfiguration in the first place.
⚠️ No test coverage for new behavior → Add test cases for formatNumber(NaN), formatNumber('hello' as any), and formatNumber('123' as any) to document and protect the new guard.
⚠️ Type signature vs actual usage → The function accepts value?: number but is called with string values from ClickHouse. The as number cast suppresses the TS error without enforcing correctness. Consider broadening the signature to value?: number | string or fixing the call sites so the type system catches future misuse.

The fix logic itself (isNaN coercion handles both NaN and non-numeric strings) is sound, but the above should be addressed before merge.

karl-power · 2026-02-23T09:20:51Z

@themavik Can you cover the isNaN check with a test case in packages/app/src/__tests__/utils.test.ts?

github-actions · 2026-02-26T10:30:51Z

Code Review

⚠️ Misleading comment → The comment says "Guard against NaN only" but isNaN('hello') is true due to JS coercion, so this also guards against non-numeric strings. Update to: "Guard against non-numeric string values (isNaN coerces strings, catching both NaN and non-parseable strings like 'hello', while numeric strings like '123' pass through to numbro)"
⚠️ Type signature mismatch → value?: number doesn't reflect actual call sites where ClickHouse returns strings; tests need @ts-expect-error to suppress it. Consider broadening to value?: number | string so the type system documents the real contract and catches future misuse without casts.
📝 NaN test cases cover pre-existing behavior → formatNumber(NaN) already returns 'N/A' via the existing \!value && value \!== 0 check (NaN is falsy). The new guard only affects non-numeric strings. The NaN tests are fine for documentation, but the real new coverage is the string test.

✅ The fix logic itself is correct and aligns with @MikeShi42's feedback — numeric strings from ClickHouse ('123') pass through to numbro while non-numeric strings ('hello') are returned as-is.

github-actions · 2026-02-26T10:32:49Z

Code Review

⚠️ Dead code path for NaN → isNaN(value as number) is unreachable for actual NaN values — the existing \!value && value \!== 0 check above already returns 'N/A' for NaN (since \!NaN === true). The new guard only ever fires for non-numeric strings like 'hello'. This means the NaN unit tests are testing pre-existing behavior, not the new code. The comment saying "Guard against NaN only" is doubly misleading.
⚠️ Misleading comment → Comment says "Guard against NaN only" but the actual new behavior is guarding non-numeric strings via JS coercion (isNaN('hello') → true). Update comment to accurately describe what's happening.
⚠️ Type signature mismatch → value?: number doesn't reflect actual call sites (ClickHouse returns numeric strings); tests require @ts-expect-error to silence TS. Consider widening to value?: number | string so the type contract is honest and the tests don't need suppressions.
ℹ️ Root cause unaddressed (noted by @karl-power, acknowledged by author) → groupByKeys in DBTableChart.tsx doesn't handle aliases like ServiceName as SN, causing numberFormat to be misapplied to non-numeric columns in the first place. This PR is a reasonable defensive patch, but a follow-up to fix alias extraction would prevent the misconfiguration entirely.

✅ The core fix logic is correct — numeric strings from ClickHouse ('123') pass isNaN and reach numbro, while non-numeric strings ('hello') return as-is. Low risk, appropriate as a patch release.

## Summary Fixes #1510 **Root cause:** When a number format (e.g., Percent) is applied to table columns that contain non-numeric values, `formatNumber` doesn't catch non-empty strings. `numbro()` then produces NaN. The existing check `!value && value !== 0` catches `null`, `undefined`, and `NaN`, but NOT non-empty strings (which are truthy and pass through to `numbro()`). ## Changes - `packages/app/src/utils.ts`: Added `typeof value !== 'number' || isNaN(value)` guard that returns the raw value as a string instead of formatting it. ## Risk Assessment **Low** - Only adds a guard for an edge case. Numeric values are formatted exactly as before. Co-authored-by: Karl Power <85935352+karl-power@users.noreply.github.com>

Merge branch 'main' into fix/number-format-nan-1510

a9d85ed

karl-power self-requested a review February 18, 2026 14:58

Merge branch 'main' into fix/number-format-nan-1510

9bd266c

karl-power reviewed Feb 20, 2026

View reviewed changes

fix: narrow formatNumber guard to isNaN only per review

3e795ad

- Remove typeof check; ClickHouse returns numbers as strings - Only guard for isNaN to catch truly non-numeric values Co-authored-by: Cursor <cursoragent@cursor.com>

Merge branch 'main' into fix/number-format-nan-1510

023aab5

themavik and others added 3 commits February 25, 2026 11:23

test: add isNaN guard test for formatNumber

c76af9f

Merge branch 'main' into fix/number-format-nan-1510

9aeb9e8

fix tests

db6440d

karl-power previously approved these changes Feb 26, 2026

View reviewed changes

karl-power added the automerge label Feb 26, 2026

add changeset

5a572ca

karl-power dismissed their stale review via 5a572ca February 26, 2026 10:31

karl-power approved these changes Feb 26, 2026

View reviewed changes

kodiakhq Bot merged commit 3797e65 into ClickHouse:main Feb 26, 2026
13 of 14 checks passed

pulpdrew mentioned this pull request Jun 4, 2026

Number format for tables should not be applied for non-numeric values (grouping keys) #1510

Closed

Navigation Menu

Repository navigation

fix: guard formatNumber against non-numeric values - #1721

fix: guard formatNumber against non-numeric values#1721
kodiakhq[bot] merged 9 commits into
ClickHouse:mainfrom
themavik:fix/number-format-nan-1510

themavik commented Feb 10, 2026

Uh oh!

changeset-bot Bot commented Feb 10, 2026 •

edited

Loading

Uh oh!

vercel Bot commented Feb 10, 2026

Uh oh!

vercel Bot commented Feb 17, 2026 •

edited

Loading

Uh oh!

MikeShi42 commented Feb 19, 2026

Uh oh!

karl-power left a comment

Uh oh!

themavik commented Feb 20, 2026

Uh oh!

github-actions Bot commented Feb 20, 2026 •

edited

Loading

Uh oh!

karl-power commented Feb 23, 2026

Uh oh!

github-actions Bot commented Feb 26, 2026

Uh oh!

github-actions Bot commented Feb 26, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

themavik commented Feb 10, 2026

Summary

Changes

Risk Assessment

Uh oh!

changeset-bot Bot commented Feb 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

vercel Bot commented Feb 10, 2026

Uh oh!

vercel Bot commented Feb 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MikeShi42 commented Feb 19, 2026

Uh oh!

karl-power left a comment

Choose a reason for hiding this comment

Uh oh!

themavik commented Feb 20, 2026

Uh oh!

github-actions Bot commented Feb 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review

Uh oh!

karl-power commented Feb 23, 2026

Uh oh!

github-actions Bot commented Feb 26, 2026

Code Review

Uh oh!

github-actions Bot commented Feb 26, 2026

Code Review

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

changeset-bot Bot commented Feb 10, 2026 •

edited

Loading

vercel Bot commented Feb 17, 2026 •

edited

Loading

github-actions Bot commented Feb 20, 2026 •

edited

Loading